Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

test: expand test coverage of events.js and fs.js #10947

Closed
wants to merge 2 commits into from
Closed

test: expand test coverage of events.js and fs.js #10947

wants to merge 2 commits into from
+38 −5

Conversation

vinimdocarmo
Copy link
Contributor

@vinimdocarmo vinimdocarmo commented Jan 22, 2017 •
edited
Loading

events.js:

  • test else path in emitMany function
  • test calling removeAllListeners() in a event emitter instance
    with no events at all
  • test calling removeListener() passing a event type that does
    not exist
  • test calling eventNames() in a event emitter instance
    with no events at all

Refs: https://coverage.nodejs.org/coverage-ba776b3a56642d4c/root/events.js.html

fs.js:

  • test reading a file passing a not valid enconding

Refs: https://coverage.nodejs.org/coverage-067be658f966dafe/root/internal/fs.js.html

Checklist
  • make -j4 test (UNIX)
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added test Issues and PRs related to the tests. dont-land-on-v7.x labels Jan 22, 2017
@mscdex mscdex added events Issues and PRs related to the events subsystem / EventEmitter. and removed dont-land-on-v7.x labels Jan 22, 2017
@vinimdocarmo vinimdocarmo changed the title test: extend test coverage of events.js test: expand test coverage of events.js and file.js Jan 23, 2017
@vinimdocarmo vinimdocarmo changed the title test: expand test coverage of events.js and file.js test: expand test coverage of events.js and fs.js Jan 23, 2017
@jasnell
Copy link
Member

jasnell commented Jan 23, 2017

It would be better to split these into two separate commits, one for each subsystem for which coverage is being improved.

@vinimdocarmo
Copy link
Contributor Author

@jasnell, I'll do it.

@vinimdocarmo
test: expand test coverage of events.js
49b0d9f 
* test else path in emitMany function
* test calling removeAllListeners() in a event emitter instance
  with no events at all
* test calling removeListener() passing a event type that does
  not exist
* test calling eventNames() in a event emitter instance
  with no events at all

Refs: https://coverage.nodejs.org/coverage-ba776b3a56642d4c/root/events.js.html
@vinimdocarmo
test: expand test coverage of fs.js
f328546 
* test reading a file passing a not valid enconding

Refs: https://coverage.nodejs.org/coverage-067be658f966dafe/root/internal/fs.js.html
@vinimdocarmo
Copy link
Contributor Author

@jasnell, done!

jasnell
jasnell approved these changes Jan 23, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! LGTM

mhdawson
mhdawson approved these changes Jan 23, 2017
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Great to see commits improving our code coverage :)

@jasnell
Copy link
Member

jasnell commented Jan 24, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/6016/

@vinimdocarmo
Copy link
Contributor Author

Any updates about this PR?

@gibfahn
Copy link
Member

gibfahn commented Jan 26, 2017

@vinimdocarmo Thanks for the ping, this PR should be good to go, as the CI failure seems unrelated:

https://ci.nodejs.org/job/node-test-binary-arm/6047/RUN_SUBSET=5,label=pi1-raspbian-wheezy/console

not ok 110 parallel/test-npm-install
  ---
  duration_ms: 180.181
  severity: fail
  stack: |-
    timeout
  ...

I'll land this tomorrow if no-one beats me to it.

@gibfahn
Copy link
Member

gibfahn commented Jan 27, 2017

Landed in b19334e and bee83e0

@gibfahn gibfahn closed this Jan 27, 2017
gibfahn pushed a commit that referenced this pull request Jan 27, 2017
@vinimdocarmo @gibfahn
test: expand test coverage of events.js
bee83e0 
* test else path in emitMany function
* test calling removeAllListeners() in a event emitter instance
  with no events at all
* test calling removeListener() passing a event type that does
  not exist
* test calling eventNames() in a event emitter instance
  with no events at all

Refs: https://coverage.nodejs.org/coverage-ba776b3a56642d4c/root/events.js.html
PR-URL: #10947
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
gibfahn pushed a commit that referenced this pull request Jan 27, 2017
@vinimdocarmo @gibfahn
test: expand test coverage of fs.js
b19334e 
* test reading a file passing a not valid enconding

Refs: https://coverage.nodejs.org/coverage-067be658f966dafe/root/internal/fs.js.html
PR-URL: #10947
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@vinimdocarmo vinimdocarmo deleted the extend-tests branch January 30, 2017 12:02
evanlucas pushed a commit that referenced this pull request Jan 31, 2017
@vinimdocarmo @evanlucas
test: expand test coverage of events.js
4aedde8 
* test else path in emitMany function
* test calling removeAllListeners() in a event emitter instance
  with no events at all
* test calling removeListener() passing a event type that does
  not exist
* test calling eventNames() in a event emitter instance
  with no events at all

Refs: https://coverage.nodejs.org/coverage-ba776b3a56642d4c/root/events.js.html
PR-URL: #10947
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
evanlucas pushed a commit that referenced this pull request Jan 31, 2017
@vinimdocarmo @evanlucas
test: expand test coverage of fs.js
cfd1b19 
* test reading a file passing a not valid enconding

Refs: https://coverage.nodejs.org/coverage-067be658f966dafe/root/internal/fs.js.html
PR-URL: #10947
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@italoacasas italoacasas mentioned this pull request Jan 31, 2017
Merged
jasnell pushed a commit that referenced this pull request Mar 8, 2017
@vinimdocarmo @jasnell
test: expand test coverage of fs.js
10e06d1 
* test reading a file passing a not valid enconding

Refs: https://coverage.nodejs.org/coverage-067be658f966dafe/root/internal/fs.js.html
PR-URL: #10947
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@jasnell
Copy link
Member

jasnell commented Mar 8, 2017

Both commits landed clean in v6. Only one (the fs.js tests) landed clean in v4. Will need a backport to land the other.

jasnell pushed a commit that referenced this pull request Mar 8, 2017
@vinimdocarmo @jasnell
test: expand test coverage of events.js
9496494 
* test else path in emitMany function
* test calling removeAllListeners() in a event emitter instance
  with no events at all
* test calling removeListener() passing a event type that does
  not exist
* test calling eventNames() in a event emitter instance
  with no events at all

Refs: https://coverage.nodejs.org/coverage-ba776b3a56642d4c/root/events.js.html
PR-URL: #10947
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
jasnell pushed a commit that referenced this pull request Mar 8, 2017
@vinimdocarmo @jasnell
test: expand test coverage of fs.js
7b040f2 
* test reading a file passing a not valid enconding

Refs: https://coverage.nodejs.org/coverage-067be658f966dafe/root/internal/fs.js.html
PR-URL: #10947
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@vinimdocarmo @MylesBorins
test: expand test coverage of events.js
5cea223 
* test else path in emitMany function
* test calling removeAllListeners() in a event emitter instance
  with no events at all
* test calling removeListener() passing a event type that does
  not exist
* test calling eventNames() in a event emitter instance
  with no events at all

Refs: https://coverage.nodejs.org/coverage-ba776b3a56642d4c/root/events.js.html
PR-URL: #10947
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@vinimdocarmo @MylesBorins
test: expand test coverage of fs.js
62f6749 
* test reading a file passing a not valid enconding

Refs: https://coverage.nodejs.org/coverage-067be658f966dafe/root/internal/fs.js.html
PR-URL: #10947
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Mar 9, 2017
@vinimdocarmo @MylesBorins
test: expand test coverage of fs.js
2f4b6bd 
* test reading a file passing a not valid enconding

Refs: https://coverage.nodejs.org/coverage-067be658f966dafe/root/internal/fs.js.html
PR-URL: #10947
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Mar 9, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@mhdawson mhdawson mhdawson approved these changes

@gibfahn gibfahn gibfahn approved these changes

Assignees
No one assigned
Labels
events Issues and PRs related to the events subsystem / EventEmitter. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vinimdocarmo @jasnell @gibfahn @mhdawson @mscdex @nodejs-github-bot